Code Climate has analyzed commit e56ecb6 and detected 0 issues on this pull request.

View more on Code Climate.

Hey @Paul-Bob . I've updated the PR to reflect the changes you requested for, as highlighted below:

• Removed the include_hidden method which caused problems on deselection. Instead, i resorted to using a before_validation method in the product model file to remove the empty string from the sizes array, which had been the purpose of using include_hidden in the first place. To ensure that this works as expected, i added a test which caters to deselection, which passes signifying that on deselecting, we get an empty array([]) as the result.
• Also i edited the existing tests and the new test to use "product_sizes" instead of "product[sizes][]"
• Made edits related to the to_permitted_param and maintained the original syntax of "aria" in the edit component file.
  Kindly review it when you get some time and get back to me about what you think.

Hey @zhephyn, thanks for the updates!

There's a bit of an issue with the sanitization. It really should be handled automatically by Avo rather than shifting that responsibility to the user.

Other than that, we're pretty much at the finish line! 🚀

I've updated the PR to reflect the changes you requested for. In particular, the ones relating to sanitization.

I've updated the PR to reflect the changes you requested for. In particular, the ones relating to sanitization.

Awesome! Let's please ensure that lint and the test actions are not failing, you can check the details for each failing action, let me know if you need any help interpreting those.

Awesome! Let's please ensure that lint and the test actions are not failing, you can check the details for each failing action, let me know if you need any help interpreting those.

The current 3 tests I had written to test for the new and edit actions relating to the field all pass as expected. The last 3 tests in the select_field_spec are the ones I'm referring to. Are there any other tests that test for this field that I should ensure "work fine"?

Hey @Paul-Bob, I've updated the PR to fix the issues that were flagged by rubocop relating to double and single quote syntax

Hey @Paul-Bob, the issues I'm seeing on checking the PR, the one for rubocop seems understandable and im gonna fix it. As for the system and feature specs, the runner is failing at bundle install and throws an argument error saying that version 8.0 is not recognized with the highest version being 7.2. This doesn't quite make sense given the fact that, locally everything works and this is a rails 8 application. The tests are being run against rails version 7.2 yet the application's migration version is 8.0.

Hey @Paul-Bob. I've updated the branch to solve the migration version error plus the double string rubocop error.
PS: Sorry about the delay

I thought the migration version had to correspond to the rails version installed by the bundle command when running the CI test. That's why I had specified version 7.1, I'm going to update the PR and use rails version 6.1 instead. Can't wait for the PR to get merged 🤞.
PS: Thank you so much for your patience. 🙏

Hey @Paul-Bob. I've updated the branch to solve the migration version error plus the double string rubocop error.
PS: Sorry about the delay

Thanks @zhephyn, no problem!

I thought the migration version had to correspond to the rails version installed by the bundle command when running the CI test. That's why I had specified version 7.1, I'm going to update the PR and use rails version 6.1 instead. Can't wait for the PR to get merged 🤞.
PS: Thank you so much for your patience. 🙏

The version must be 6.1 or lower since that's the minimum version used for testing.

I've observed some test failures, could you please investigate those?

The tests are failing because rails possibly has no inbuilt provision for allowing multiple values for an enum declaration in a model. Rails expects that an enum should allow single and not multiple selection.
In the project model file, we define an enum called "Stage" and provide the options including "On hold, cancelled, drafting" and so on. Then in the schema.rb, we can see that the stage column of the projects table, has array set to "true", which is meant to say that it can accept an array/multiple values instead of a single value. However, in factories.rb, for tests that rely on creating a project first before the test is run, the stage field is populated with a single value instead of an array, which is why we see the 'Malformed array literal" error. To solve this we could EITHER,

• Do away with the enum altogether in the project model file and specify the options for the stage field in the project.rb resource file like we did for the "Sizes" column of the product table, similar to this:
  field :type, as: :select, options: { 'Large container': :large, 'Medium container': :medium, 'Tiny container': :tiny }, display_with_value: true, placeholder: 'Choose the type of the container.'
• OR we could manipulate the values sent after populating the "Stage" field in the project.rb model file, before saving which i think could allow us to keep the enum declaration.
• Another option would be to run a migration to change the Stage column to a non-array column. Though if my memory serves me right, initially the issue was related to making this particular field a multiple select.

Kindly let me know which approach is you'd prefer and i'll get to solving the error ASAP!

initially the issue was related to making this particular field a multiple select.

The spec/dummy directory contains the local application used for development and testing. The issue is about implementing the multiple option for select fields. All changes made to spec/dummy should be exclusively for testing this feature.

I mentioned that specific field in the video to illustrate how multiple select should work. However, it’s not necessary to modify that exact field or data structure. Since that field is already being used in other tests, it’s preferable to create a new field specifically for testing the multiple select use case, similar to how you created the sizes field for products.

Let me know if you have any questions.

Thanks for the feedback. As for what I meant by the statement, initially what we were aiming for is that for every resource that has a multiple select, whether it's Product or Project, any field of this resource can use the select option which can allow multiple options to be selected. And so far, we implemented it correctly for the sizes column of the products table. All the tests related to this pass. However, the problem came with the fact that the stage column of the projects table is set to accept array values, yet the Enumerable declaration for the stage in the project model doesn't allow this. This is a missing feature when working with Postgres backed columns in rails. The enum can't allow multiple values but rather a single value. Which is why those tests failed. And this is why I had suggested as one of the approaches that we could remove the "array: true" option from the stage column by running a migration, this will solve the issue. However, this would defeat the initial suggestion since you had said that we should make changes such that the we can select multiple stages whether two or more for a given project. By removing the "array: true", this wouldn't be possible. Should I create a demo with code and perhaps video to show what I'm referring to approach wise? Please let me know. Thanks for the feedback. I'm very grateful for the learning experience 🙏

@zhephyn, can we hop on a quick call to talk this through?

@zhephyn, can we hop on a quick call to talk this through?

Sure. Can we use zoom? Or there's another alternative?

Hey @Paul-Bob. After some heavy tinkering, i have come to understand what could have caused the test to fail on rails 6. A potential solution is adding the to_permitted_param in the tiptap_field.rb file like below:
def to_permitted_param [:sizes] end
This works but then the tests fail by throwing a "unpermitted parameter :description error" probably because of hardcoding ":sizes" in the method. Is there a way we can edit the above to_permitted_param method and make it more dynamic, kinda like what we did in the select_field.rb file, to end up with @multiple instead of hard coding the ":sizes" field, since with the hard coding approach , we run into another error?
Thanks

Thanks for the context @zhephyn

I'll have a look

Hey @Paul-Bob . I've seen the updates you pushed to the PR and that all the tests are passing successfully. Does it mean there is no more work to be done on this PR?

There was an issue with how the multiple option was configured on the select helper. I modified it to ensure compatibility with Rails 6.1.

A test is required for the show view, which I fixed in a previous commit, along with documentation updates. Could you please take care of those?

I'd be glad to.
A few clarifications though,

1. For the show view, is a new test to be added to the select field spec? And what are we testing for? I've tried out the feature in the browser and the empty string which was causing issues initially is removed before saving happens.
2. Are there any guidelines regarding how to go about updating the docs, I've never pushed an update to them before? Plus, is this update being added to the select_field documentation?
   Thanks

For the show view, is a new test to be added to the select field spec? And what are we testing for?

Yes is a new test.

We're testing that when DB value is ["small", "large"] the field component on the show view is displayed as Small, Large and when display_value is set true the show component for the select field display ["small", "large"].

Are there any guidelines regarding how to go about updating the docs, I've never pushed an update to them before? Plus, is this update being added to the select_field documentation?

There is a contribution guide on the docs repo

Don't worry about the docs, I'll do the docs

Thank you